Clean up various autoscheduler tool issues #7483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- In CMake, not all of the tooling for Anderson2021 was included in packaging; now it is. - Since the featurization_to_sample and get_host_target tools are 100% identical across all autoschedulers, they now get build only once, and don't get the `adams2019_` (etc) prefix they formerly did. - conversely, the autotune_loop.sh and weightsdir_to_weightsfile tools now *do* get autoscheduler-specific prefixes, because they aren't interchangeable. - Fixes to various scripts etc to use the correct tool names. - When the Anderson2021 autoscheduler was added, we tried to factor out common coding and tooling, but went a bit too far: the weightsdir_to_weightsfile source was moved into common/, and while it and the related Weights source were identical, they needed to include *different* Featurization.h files, and so basically relied on the build rules building it twice, with a different include path each time. IMHO this is unreasonably fragile and weird, so I moved those sources back into the folders in question, as there isn't any compelling reason to keep these sources exactly in sync anyway. - Same story for the test_function_dag test. Note that I started to add support for Anderson2021 to the main Makefile, but it became painful to do -- it works fine for CMake, so I will leave adding Make support for someone who wants to use it there.
steven-johnson
added a commit
that referenced
this pull request
Apr 6, 2023
The Halide 15.0.0 release inadvertently omitted all of these tools from the packaging. They are being re-added for 15.0.1. While we're doing this, let's go ahead and have the names match those proposed for Halide 16.
steven-johnson
added a commit
that referenced
this pull request
Apr 6, 2023
abadams
reviewed
Apr 7, 2023
| @@ -36,23 +36,23 @@ $(BIN)/baseline_weights.o: $(BIN)/baseline_weights.cpp | |||
| $(CXX) -c $< -o $@ | |||
|
|
|||
| AUTOSCHED_COST_MODEL_LIBS=\ | |||
| $(BIN)/cost_model/adams2019_cost_model.a \ | |||
| $(BIN)/cost_model/adams2019_train_cost_model.a \ | |||
| $(BIN)/adams2019_cost_model/adams2019_cost_model.a \ | |||
There was a problem hiding this comment.
Why the adams2019 prefix on the cost model directory and generator? This file should be private to this autoscheduler, so it shouldn't need namespacing,
There was a problem hiding this comment.
Ah, a holdover from a brief, aborted attempt at integrating Anderson2021 into the base Makefile -- both this and that were trying to create the same target, and hilarity ensued. I'll revert them.
abadams
reviewed
Apr 7, 2023
| @@ -14,7 +14,7 @@ int main(int argc, char **argv) { | |||
| AutoschedulerParams params = {"Anderson2021", {{"parallelism", std::to_string(hardware_parallelism)}}}; | |||
|
|
|||
| // Use a fixed target for the analysis to get consistent results from this test. | |||
| Target target("x86-64-linux-sse41-avx-avx2-cuda"); | |||
| Target target("x86-64-linux-sse41-avx-avx2-cuda-debug"); | |||
There was a problem hiding this comment.
Unintentional? This isn't going to change anything in the current test, but if the test is changed to actually run code, it's going to invalidate any performance measurements.
There was a problem hiding this comment.
oops, yes, fixing
abadams
reviewed
Apr 7, 2023
|
Monday Morning Review Ping |
|
LGTM for my part, but would like Alex to approve for the cmake changes. |
alexreinking
approved these changes
Apr 11, 2023
ardier
pushed a commit
to ardier/Halide-mutation
that referenced
this pull request
Mar 3, 2024
* Clean up various autoscheduler tool issues - In CMake, not all of the tooling for Anderson2021 was included in packaging; now it is. - Since the featurization_to_sample and get_host_target tools are 100% identical across all autoschedulers, they now get build only once, and don't get the `adams2019_` (etc) prefix they formerly did. - conversely, the autotune_loop.sh and weightsdir_to_weightsfile tools now *do* get autoscheduler-specific prefixes, because they aren't interchangeable. - Fixes to various scripts etc to use the correct tool names. - When the Anderson2021 autoscheduler was added, we tried to factor out common coding and tooling, but went a bit too far: the weightsdir_to_weightsfile source was moved into common/, and while it and the related Weights source were identical, they needed to include *different* Featurization.h files, and so basically relied on the build rules building it twice, with a different include path each time. IMHO this is unreasonably fragile and weird, so I moved those sources back into the folders in question, as there isn't any compelling reason to keep these sources exactly in sync anyway. - Same story for the test_function_dag test. Note that I started to add support for Anderson2021 to the main Makefile, but it became painful to do -- it works fine for CMake, so I will leave adding Make support for someone who wants to use it there. * Fix needless renames * Remove scalpel left in patient * Update CMakeLists.txt * Update Makefile
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adams2019_(etc) prefix they formerly did.Note that I started to add support for Anderson2021 to the main Makefile, but it became painful to do -- it works fine for CMake, so I will leave adding Make support for someone who wants to use it there. (Honestly, the Makefile for it is still kind of a mess even after some cleanup; I'm tempted to delete it and just declare that that one is CMake-only...)